Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

25167 add feature flags service enable won emails #1659

Merged

Conversation

kzdev420
Copy link
Contributor

@kzdev420 kzdev420 commented Jan 9, 2025

Issue #, if available: #25167

Description of changes:

Add Feature Flags service.
Use the enable way of navigation emails FF

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namex license (Apache 2.0).

api/.env.sample Outdated Show resolved Hide resolved
@EPortman
Copy link
Collaborator

EPortman commented Jan 9, 2025

Once this is ready to be merged the emailer needs to install the latest API changes.

If simply running poetry update namex in the emailer repo is not working I would recommend doing this:

  1. Find the commit hash you want the emailer to use
  • Run git log it will be the first one if you want the latest changes (in this PR)
  • Grab the commit hash that it gives (e.g. 4d7464e1a331f53736a5823f71dcde10a5ef2d07)
  1. Update the lock file with this commit hash

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the Feature Flag is to:

  • if True, send out the new emails
  • if False, send out the old emails

This needs to apply for all "way of navigation" emails -- NEW and CHG/CNV.

I think I don't see both the old and new code, conditional on the FF, in this PR.


Update: Kevin walked me through this. In a nutshell, some duplicate code was removed as it is now directly available in Namex API.

I think this is fine.

@severinbeauvais
Copy link
Collaborator

@kzdev420 Are there new linting and testing issues?

image

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please double-check per my comments above.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
10 Security Hotspots
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Jan 10, 2025

OK, @EPortman and @kzdev420 , I'm leaving this with you to merge and deploy to Dev.

Then, please test this in Dev. Currently, the FF is On (true). Tomorrow, we'll test again with the FF = Off (false).

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@rarmitag rarmitag merged commit a45b97f into bcgov:main Jan 10, 2025
3 of 6 checks passed
@@ -81,3 +81,6 @@ MRAS_SVC_API_KEY=

# Local development only
DISABLE_NAMEREQUEST_SOLR_UPDATES=1

# launchdarkly
NAMEX_LD_SDK_ID=sdk-075200eb-4ff7-4e0e-872a-848585d3d460
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kzdev420
I bet this key has to be imported in api/config.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants